-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try to coerce all objects to Numpy arrays. #1393
Conversation
Hi @malmaud , Thanks for the PR! Here are a couple of thoughts
I haven't looked into this too much yet, but do PyTorch tensors and xarray DataArrays support the numpy array interface? (https://docs.scipy.org/doc/numpy-1.15.1/reference/arrays.interface.html). If so, we might be able to handle this usecase across all array types by adding a simple check to |
Thanks for the feedback! I'll implement 1) and 2).
I think just checking for |
👍 |
OK, I made the changes. |
Thanks @malmaud, In terms of testing, it looks like something went wrong in
Before merging this, we'll also need some tests that exercise the new
Are you interested in adding these tests? If not, we can just leave the PR open until I have a chance to go through and add them (which would hopefully be by the end of the month). |
Alright, I'll take a look and add the tests. |
Tests have been added. I had to make some other changes to make the tests pass, so apparently that was a worthwhile exercise. |
@@ -43,12 +43,16 @@ def fullmatch(regex, string, flags=0): | |||
# Utility functions | |||
# ----------------- | |||
def to_scalar_or_list(v): | |||
if np and np.isscalar(v) and hasattr(v, 'item'): | |||
return np.asscalar(v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this branch. What kinds of objects does this handle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a comment to the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
tox.ini
Outdated
@@ -72,7 +72,7 @@ deps= | |||
optional: pyshp==1.2.10 | |||
optional: pillow==5.2.0 | |||
optional: matplotlib==2.2.3 | |||
optional: xarray==0.11.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for future reference, what kind of problem did this cause?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the tests reported that 0.10.9 was the highest version of xarray
available. I guess xarray
added some upper bound to its dependencies in the 0.11 series that rendered it incompatible with the environment of the test - I didn't really investigate too closely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info. I figured it was a transitive dependency issue, just wanted to double check.
This is great work @malmaud, thanks for taking this on and especially for adding the tests! I had two inline questions, but everything else lgtm. |
Merged! Thanks again for the solid PR. |
Please see #1427 |
This lets you pass a broad array of list-like objects as data to be plotted instead of the current hard-coded list of numpy arrays, lists, tuples, and Pandas objects. Objects such as PyTorch tensors (widely used in the machine-learning world), xarrays, and others know how to convert themselves into Numpy arrays and so might as well be allowed to be passed directly as plotting arguments so users don't have to explicitly convert them.